Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merging common code bases DbConnectionStringCommon #1265

Merged
merged 8 commits into from
Oct 12, 2021
Merged

Merging common code bases DbConnectionStringCommon #1265

merged 8 commits into from
Oct 12, 2021

Conversation

DavoudEshtehari
Copy link
Contributor

Common types:

  1. DbConnectionStringDefaults
  2. DbConnectionStringKeywords
  3. DbConnectionStringSynonyms
  4. DbConnectionStringBuilderUtil

@DavoudEshtehari DavoudEshtehari added the ➕ Code Health Issues/PRs that are targeted to source code quality improvements. label Sep 15, 2021
@DavoudEshtehari DavoudEshtehari added this to the 4.0.0-preview2 milestone Sep 15, 2021
@DavoudEshtehari DavoudEshtehari mentioned this pull request Sep 15, 2021
Open
@Wraith2
Copy link
Contributor

Wraith2 commented Sep 15, 2021

What s the reason for a dedicated ReadOnlyCollection<T> type? Can we not use the standard bcl provided one? https://apisof.net/catalog/9ff7feaa-ce3f-c2d6-3b5d-4cf21ce96643 and if not can there be a comment explaining the behaviour we need from our own version in the code?

@DavoudEshtehari DavoudEshtehari linked an issue Sep 15, 2021 that may be closed by this pull request
SqlConnectionStringBuilder should also be updated to match the default value changes in 4.0.0-preview1 #1248
Closed
@DavoudEshtehari
Copy link
Contributor Author

What s the reason for a dedicated ReadOnlyCollection<T> type? Can we not use the standard bcl provided one? https://apisof.net/catalog/9ff7feaa-ce3f-c2d6-3b5d-4cf21ce96643 and if not can there be a comment explaining the behaviour we need from our own version in the code?

Thank you for bringing it up. I didn't find any reason for using ReadOnlyCollection<T> too. Just brought it here for more discussion.
cc @cheenamalhotra

@cheenamalhotra
Copy link
Member

What s the reason for a dedicated ReadOnlyCollection<T> type? Can we not use the standard bcl provided one? https://apisof.net/catalog/9ff7feaa-ce3f-c2d6-3b5d-4cf21ce96643 and if not can there be a comment explaining the behaviour we need from our own version in the code?

Thank you for bringing it up. I didn't find any reason for using ReadOnlyCollection<T> too. Just brought it here for more discussion.
cc @cheenamalhotra

I agree we could use the standard BCL type already available.

@DavoudEshtehari
Merge remote-tracking branch 'UpSream/main' into Adhoc/MergeCodeCommon1
12c0c06 
# Conflicts:
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj
#	src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/DbConnectionOptions.cs
@DavoudEshtehari
Address comment
c03e549 
remove _emptyString
syntax improvements
johnnypham
johnnypham suggested changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@johnnypham johnnypham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old netcore file can be deleted.

@cheenamalhotra cheenamalhotra merged commit 6c46392 into dotnet:main Oct 12, 2021
@cheenamalhotra cheenamalhotra mentioned this pull request Oct 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wraith2 Wraith2 Wraith2 left review comments

@johnnypham johnnypham johnnypham approved these changes

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

@JRahnama JRahnama Awaiting requested review from JRahnama

@Kaur-Parminder Kaur-Parminder Awaiting requested review from Kaur-Parminder

Assignees
No one assigned
Labels
➕ Code Health Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Milestone
4.0.0-preview3
Development

Successfully merging this pull request may close these issues.

SqlConnectionStringBuilder should also be updated to match the default value changes in 4.0.0-preview1
4 participants
@DavoudEshtehari @Wraith2 @cheenamalhotra @johnnypham